Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesMatcher output truncation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/test_runner/mod.rs`:
- Around line 382-386: Define a named constant for the 1 MiB matcher output
budget near the relevant test-runner configuration, then assign that constant to
f.max_output_bytes instead of using the literal shift. Keep the limit unchanged
and use a descriptive name that identifies it as the matcher output byte limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbeaf424-4cf2-44e2-9e48-bc920c5a3c4e
📒 Files selected for processing (4)
src/jsc/ConsoleObject.rssrc/jsc/JSGlobalObject.rssrc/runtime/test_runner/mod.rstest/js/bun/test/expect-failure-message-truncation.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate. #34179 caps the diff formatter used by the toEqual family (JestPrettyFormat / DiffFormatter, issue #34178). This PR caps the inline value formatter (ConsoleObject::Formatter) used by simple matchers like toBeNull, which #34179 explicitly left alone on the assumption that its depth bound was enough; #37310 shows a wide graph blows up on breadth despite the depth bound, and past the max JS string length the assertion error is silently lost. The two changes touch different code paths and are complementary. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bun_core/util.rs`:
- Around line 1753-1760: Implement is_truncated() forwarding in the Write
implementations for &mut W and Box<W>, delegating each wrapper’s result to its
contained writer. Preserve the existing default behavior for other writers so
truncation state from TruncatingWriter reaches print_as_prelude through both
wrapper types.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3030f863-8eb3-4f7c-b4f8-c657c378dc89
📒 Files selected for processing (4)
src/bun_core/util.rssrc/jsc/ConsoleObject.rssrc/runtime/test_runner/mod.rstest/js/bun/test/expect-failure-message-truncation.test.ts
There was a problem hiding this comment.
I reviewed the latest revision and found no new issues — d7c1367 addresses the last remaining sibling sites in ScopeFunctions.rs, and all earlier findings (the raw-sink panic, the truncation-masking-a-pending-exception case, the is_truncated() wrapper forwarding, and the uncapped format_value/snapshot/toThrow paths) are resolved. Given the change touches JSGlobalObject::throw, the shared bun_io::Write trait, and the console formatter's core render loop, a human pass on the overall design (1 MiB cap value, the trait method addition, and the fallback error wording) would still be worthwhile.
What was reviewed:
TruncatingWriternever returnsErr, so the ~30.expect("unreachable")raw-sink sites inConsoleObject.rsremain sound;print_as_preludepollsis_truncated()to stop the walk.- The
if truncated && result.is_ok()guard preserves the pre-PR contract that a pending JS exception from a getter still propagates tocreate_error_instance's clear-and-throw path. - Grepped every
Formatter::newinsrc/runtime/test_runner/— all sites now either chain.with_quote_strings()(which sets the cap), go throughmake_formatter, or setmax_output_bytesdirectly;pretty_format.rsis intentionally deferred to #34179. format_valuenow honorsmax_output_bytesfor thetest.each%p/$varlabel path.
Extended reasoning...
Overview
This PR fixes #37310: expect(hugeObject).toBeNull() rendered the received value into the failure message with no output limit, so a wide object graph (e.g. a happy-dom DOM tree) expanded to gigabytes; past WTF::String::MaxLength the error string could not be materialized, create_error_instance returned empty with no pending exception, and JSGlobalObject::throw returned JsError::Thrown with nothing on the VM — a failed assertion silently passed.
The fix has two parts: (1) a byte-budgeted TruncatingWriter sink wired through the console formatter via a new max_output_bytes field (default unlimited; the test runner's matcher formatters set it to 1 MiB), with a new defaulted is_truncated() method on bun_io::Write that print_as_prelude polls to stop the value walk; and (2) a defensive fallback in JSGlobalObject::throw so an empty error instance with no pending exception throws a short placeholder instead of silently succeeding.
Files touched: src/bun_core/util.rs (Write trait), src/jsc/ConsoleObject.rs (formatter core, ~100 lines), src/jsc/JSGlobalObject.rs (throw), three test-runner files applying the cap at every formatter construction site, and a new 88-line test file covering object graphs, huge strings, huge Maps, toThrow, the getter-throws-after-cap edge case, and a small-value negative case.
Security risks
None identified. The change bounds output rather than expanding it, and the throw fallback fails closed (throws a generic error) rather than open. No user input reaches new parsing or allocation paths.
Level of scrutiny
Medium-high. ConsoleObject.rs is the shared value formatter used by console.log, Bun.inspect, and every matcher failure message — a subtle regression here would be broadly visible. JSGlobalObject::throw sits on every Rust-side throw path. The change is careful to preserve defaults (max_output_bytes = usize::MAX for non-test-runner callers; TruncatingWriter never returns Err so the formatter's infallible-write invariants hold), and the existing console/inspect/expect suites pass per the PR evidence, but the surface is critical enough that a maintainer should confirm the design choices — in particular the addition of a defaulted method to the public bun_io::Write trait, the 1 MiB cap value, and the fallback error message wording.
Other factors
This PR went through three prior review rounds from me, each of which surfaced a real issue that was then fixed: the initial TruncatingWriter returned Err and panicked ~30 raw-sink .expect("unreachable") sites on large strings/Maps; the truncation branch discarded a pending JS exception from a post-cap getter; is_truncated() was not forwarded through &mut W/Box<W>; and successive rounds enumerated remaining uncapped sibling sites (with_quote_strings callers, format_value, three snapshot-error arms in expect.rs, and finally the two .each() error paths in ScopeFunctions.rs). All are now addressed and covered by the new tests. The DiffFormatter/JestPrettyFormat path is explicitly deferred to the complementary #34179. This run's bug hunt found nothing new; I re-grepped every Formatter::new in the test runner and confirmed the class is closed modulo the documented exclusion. The number of iterations it took to converge is itself a signal that the change is non-trivial and merits a human glance.
|
CI summary for e74ad10: 195 of 196 jobs passed. The one failed job (darwin 14 x64 test-bun) died during git checkout with "Could not resolve host: github.com" on the agent, before any tests ran. The new truncation tests passed on every lane that ran, and the two flaky annotations (bun-add git URL timeout on alpine, passed on retry) are unrelated to this change. The diff is ready for review. |
Rendering the received value into an expect() failure message was unbounded. A wide object graph (e.g. a happy-dom DOM tree) expanded to gigabytes of text, and once the rendered message exceeded the maximum JS string length the Error instance could not be created, so the failed assertion returned without throwing and the test passed. Cap each value rendered into a matcher failure message at 1 MiB. The budgeted sink discards past the cap and reports is_truncated(), a new default method on the Write trait; print_as_prelude polls it to stop the walk, and the message ends with a '... [value truncated]' marker. The truncation branch only rewrites an Ok result so a pending exception from a getter still propagates. Also make JSGlobalObject::throw fall back to a short static message when error instance creation fails with no pending exception. Fixes #37310
e74ad10 to
8425d62
Compare
|
Updated 1:43 PM PT - Sep 12th, 2026
✅ @robobun, your commit e41fa6a6b91f72d7e147d397d88354b6dc062957 passed in 🧪 To try this PR locally: bunx bun-pr 37311That installs a local version of the PR into your bun-37311 --bun |
There was a problem hiding this comment.
I reviewed the latest revision and didn't find any bugs — all prior findings from earlier rounds (the .expect("unreachable") panic, is_truncated() forwarding through &mut W/Box<W>, the truncated && result.is_ok() guard for pending getter exceptions, and the remaining bare-Formatter::new sites in expect.rs and ScopeFunctions.rs) are addressed. Because this extends the bun_io::Write trait, changes the JSGlobalObject::throw fallback semantics, and reroutes every format_value caller through TruncatingWriter, a human sign-off is still worthwhile.
The one open comment-cop flag on ConsoleObject.rs:5762 is a two-line explanatory comment, consistent with the earlier instances the author already addressed.
What was reviewed
TruncatingWriternow discards past the cap and never errors, so the ~30 raw-sink.expect("unreachable")sites remain sound;print_as_preludepollsis_truncated()to stop the walk.ZigFormatter::fmtalready routed throughformat_value, so moving theTruncatingWriterwrap there covers both the Display path and the directjest.rs/expect.rscallers with one change;console.log/Bun.inspectinheritusize::MAXand are unaffected.- The
throw()fallback only fires on empty-instance-with-no-pending-exception;throw_value's existinghas_exception()guard is preserved.
Extended reasoning...
Overview
The PR fixes #37310: expect(hugeValue).toBeNull() rendered an unbounded failure message, and past WTF::String::MaxLength the assertion error could not be created, so the failed assertion silently passed. The fix adds a max_output_bytes budget to ConsoleObject::Formatter, wraps the sink in a discarding TruncatingWriter inside format_value, polls is_truncated() in print_as_prelude to stop the graph walk, and hardens JSGlobalObject::throw to throw a fallback error when create_error_instance returns empty with no pending exception. Every test-runner formatter construction site (make_formatter, with_quote_strings, and the bare Formatter::new sites in expect.rs/ScopeFunctions.rs) now sets the 1 MiB cap.
Security risks
None. The change bounds output size (strictly reduces resource consumption) and ensures a throw is never silently dropped. No untrusted-input parsing, auth, or crypto is touched.
Level of scrutiny
Medium-high. The change is behaviorally narrow (default usize::MAX leaves console.log/Bun.inspect unchanged), but it touches three widely-shared surfaces: it adds a defaulted method to the foundational bun_io::Write trait, changes the shared format_value entry point that both ZigFormatter (Display) and direct callers route through, and adds a fallback branch to JSGlobalObject::throw. Each of these has many downstream consumers.
Other factors
This PR went through four review rounds, each of which surfaced a real defect that was then fixed (raw-sink panic, missing wrapper forwarding, swallowed getter exception when truncated && Err, and three successive rounds of missed sibling Formatter::new sites). The final design is clean — the TruncatingWriter wrap lives in the single format_value choke point rather than duplicated at each entry — and the test file covers object graphs, huge strings, Maps, toThrow, a throwing getter after the cap, and the small-value negative case. CI passed 195/196 (one unrelated agent network failure). Given the number of iterations needed to converge and the breadth of the touched infrastructure, a maintainer sign-off on the trait extension and throw() fallback is appropriate.
…message-truncation # Conflicts: # src/runtime/test_runner/expect.rs
…message-truncation
main no longer has ZigString in JSGlobalObject.rs.
|
Updated this PR to current main (it was 594 commits behind).
Verified on a debug ASAN build of the merged tree: #34179 (the diff and snapshot formatter) no longer touches |
|
Closing in favor of #44833, which fixes #37310 on canary by printing a DOM node in a failure message as markup, so a failing toBeNull() stays about a millisecond at every tree size. That is a targeted fix for the reported case, and the lost throw was already gone in v1.4.4. This PR took the broader approach of a generic 1 MiB cap on all matcher output, which is not needed now that the specific case is handled and is a behavior change (truncating failure messages) that was not requested. Thanks for the pointer to #44833. |
Fixes #37310
Problem
expect(<happy-dom Element>).toBeNull()on a large document got superlinearly slower, and past a size threshold it stopped throwing entirely, reporting a false assertion as a passing test.Two compounding bugs:
Zig::getErrorInstancereturned an empty value with no pending exception,JSGlobalObject::throwturned that intoJsError::Thrownwith nothing actually thrown, and the host function returned normally. The failed assertion silently passed.Repro (from the issue, happy-dom 20.x):
Fix
max_output_bytes, default unlimited). The test runner's matcher formatter caps each rendered value at 1 MiB; when the budget is hit, the value walk stops and the message ends with... [value truncated]. This bounds both the rendering time and the message size, so the message can always be materialized as a JS string.console.log,Bun.inspect, and other formatter users are unchanged.JSGlobalObject::throw: if the error instance could not be created and no exception is pending, throw a short fallback error instead of returningThrownwith nothing thrown. An assertion failure (or any throw) can no longer be silently dropped by message construction failing.Verification
New test
test/js/bun/test/expect-failure-message-truncation.test.ts: a ~41k-node object tree used to render a multi-MB failure message; now the message is capped and the assertion still throws. Fails on current main, passes with this change.The issue's verbatim happy-dom repro, with the fix (debug + ASAN build):
Every size throws, and the cost is flat instead of superlinear (previously 100s+ in a release build at fanout 120, with the throw lost past fanout 80).
Existing suites pass:
expect.test.js(415 pass),test/js/bun/console(85 pass),test/js/bun/util/inspect.test.js(74 pass).Rebase note: #36912 on main refactored
ZigFormatter::fmtto delegate toFormatter::format_value. The cap for that path now lives entirely informat_value, which this PR already instrumented, soZigFormatter::fmtkeeps main's shape with no truncation code of its own. The branch was squashed to one commit during the rebase. All suites re-ran green after the rebase, including theexpect-extend-matcher-utils-throwtest that #36912 added.[human-review] gate passed · iteration 4 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 0 rejected · iteration 4
evidence per changed file
root cause · written by the author bot
The failure message for
expect(value).toBeNull()was built by walking the entire value graph with no size limit, so a DOM element rooted in a large document produced output that grew superlinearly and, once it exceededWTF::String::MaxLength, could not be turned into an error instance, leaving nothing to throw and letting the false assertion pass silently. The fix gives the formatter a byte budget, wraps its sink in aTruncatingWriterthat discards output past the cap while preserving valid UTF-8 and appends a truncation marker, and stops the graph walk as soon as truncation is detect…